Skip to content

[TT-16980] Fix pgx v5 breaking change when postgres.prefer_simple_protocol is set to true#970

Closed
buraksezer wants to merge 3 commits intomasterfrom
fix/TT-16980/fix-pgx-month-conversion
Closed

[TT-16980] Fix pgx v5 breaking change when postgres.prefer_simple_protocol is set to true#970
buraksezer wants to merge 3 commits intomasterfrom
fix/TT-16980/fix-pgx-month-conversion

Conversation

@buraksezer
Copy link
Copy Markdown
Contributor

@buraksezer buraksezer commented Apr 17, 2026

PR for https://tyktech.atlassian.net/browse/TT-16980

The proposed solution(BeforeCreate + SetColumn) didn't work because GORM's ConvertToCreateValues reads field values via field.ValueOf(), which uses reflection to return fieldValue.Interface(). Since the struct field type is time.Month, the value always reaches pgx as time.Month regardless of what SetColumn wrote. Go's reflection preserves the named type.

Another possible solution might be to change the data type of Month field in AnalyticsRecord. The current type is time.Month, if we create a new new type that implements driver.Valuer but it is a potential breaking change and requires update across all Tyk components other potentials dependents.

Implemented solution: In pumps/sql.go, I replaced the postgres case in Dialect() to build the *sql.DB ourselves via stdlib.OpenDB with an OptionAfterConnect callback.

On each new connection, we prepend a custom TryWrapEncodePlanFunc to the encode plan chain that intercepts time.Month and converts it to int before pgx's fmt.Stringer wrapper can fire. I also replicate the timezone extraction logic from gorm.io/driver/postgres to avoid behavioral regression, and pass the pre-built *sql.DB to GORM via postgres.Config{Conn: sqlDB}.

I have also added a test named TestSQLWriteData_PreferSimpleProtocol_Month, before the fix that test was failing.

How to run tests:

TYK_TEST_POSTGRES="host=127.0.0.1 port=5432 user=postgres password=postgres dbname=postgres_test sslmode=disable" go test ./pumps/ -run TestSQLWriteData_PreferSimpleProtocol_Month -v

@probelabs
Copy link
Copy Markdown
Contributor

probelabs Bot commented Apr 17, 2026

This PR addresses a breaking change from the pgx/v5 library upgrade that caused database insertion failures for PostgreSQL when prefer_simple_protocol is enabled. The issue stemmed from time.Month values being incorrectly encoded as strings (e.g., "May") instead of their underlying integer representation.

The solution avoids modifying shared data structures and instead operates at the database driver level. The Dialect function for PostgreSQL in pumps/sql.go has been refactored to manually initialize the database connection using pgx. It intercepts the connection process with an AfterConnect hook to prepend a custom encoding plan to the pgx type map. This new plan ensures that time.Month values are converted to integers before being passed to the database, resolving the serialization conflict. The implementation also preserves the existing timezone handling from the GORM driver to prevent regressions.

To validate the fix, a new integration test, TestSQLWriteData_PreferSimpleProtocol_Month, has been added. This test specifically configures the SQL pump with prefer_simple_protocol: true, writes a record containing a time.Month value, and verifies that it can be successfully persisted and retrieved.

Files Changed Analysis

  • go.mod / go.sum: Upgraded github.com/jackc/pgx/v5 to v5.9.1 and updated github.com/stretchr/testify along with other dependencies to resolve the issue and support the new test.
  • pumps/sql.go: Contains the core logic. The Dialect function for the postgres case is modified to manually build the *sql.DB connection. This allows for the injection of a custom type encoder for time.Month via an AfterConnect hook before the connection is passed to GORM.
  • pumps/sql_test.go: A new test, TestSQLWriteData_PreferSimpleProtocol_Month, is introduced to reproduce the original bug and confirm that the fix correctly handles time.Month serialization with the simple protocol.

Architecture & Impact Assessment

  • What this PR accomplishes: It resolves a critical data serialization bug, ensuring analytics data can be reliably written to PostgreSQL with prefer_simple_protocol enabled after the pgx/v5 upgrade.
  • Key technical changes introduced:
    • Manual initialization of the PostgreSQL database connection using pgx's stdlib.OpenDB.
    • Injection of a custom EncodePlan into the pgx type map on each new connection to handle time.Month to int conversion.
    • The pre-configured *sql.DB connection pool is passed to the GORM driver, overriding its default connection logic.
  • Affected system components: The change is isolated to the SQLPump when configured with a PostgreSQL backend and pumps.sql.postgres.prefer_simple_protocol set to true.
  • Data Flow Visualization:

sequenceDiagram
participant App as Tyk Pump
participant GORM
participant PGX Driver
participant PostgreSQL DB

App->>GORM: Write(AnalyticsRecord{Month: time.May})
GORM->>PGX Driver: Execute INSERT
Note over PGX Driver: AfterConnect hook has already
Note over PGX Driver: prepended custom encoder
PGX Driver->>PGX Driver: Encode(time.May) results in int(5)
PGX Driver->>PostgreSQL DB: INSERT INTO tyk_analytics (..., month, ...) VALUES (..., 5, ...)
PostgreSQL DB-->>PGX Driver: Success
PGX Driver-->>GORM: Success
GORM-->>App: Success

## Scope Discovery & Context Expansion
- The fix is narrowly scoped to the PostgreSQL dialect initialization within `pumps/sql.go`. It does not affect other SQL dialects (e.g., MySQL) or other pumps.
- The primary data structure involved is `analytics.AnalyticsRecord`, specifically its `Month time.Month` field.
- By implementing the fix at the database driver level, the solution is transparent to higher-level application logic. This is a significant architectural advantage, as it avoids a broader, potentially breaking change to the `AnalyticsRecord` struct, which could have impacted numerous other Tyk components.


<details>
  <summary>Metadata</summary>

  - Review Effort: 3 / 5
  - Primary Label: bug


</details>
<!-- visor:section-end id="overview" -->

<!-- visor:thread-end key="TykTechnologies/tyk-pump#970@a66909a" -->

---

*Powered by [Visor](https://probelabs.com/visor) from [Probelabs](https://probelabs.com)*

*Last updated: 2026-04-17T12:06:08.111Z | Triggered by: pr_updated | Commit: a66909a*

💡 **TIP:** You can chat with Visor using `/visor ask <your question>`
<!-- /visor-comment-id:visor-thread-overview-TykTechnologies/tyk-pump#970 -->

@probelabs
Copy link
Copy Markdown
Contributor

probelabs Bot commented Apr 17, 2026

✅ Security Check Passed

No security issues found – changes LGTM.

Architecture Issues (1)

Severity Location Issue
🟡 Warning pumps/sql.go:130-132
The custom PostgreSQL connection logic manually replicates DSN parsing (specifically for timezone extraction) from the `gorm.io/driver/postgres` library. While necessary for this fix, it creates a maintenance dependency. If the GORM driver's DSN parsing logic changes in a future version, this implementation will become outdated and could lead to behavioral inconsistencies.
💡 SuggestionTo mitigate this, add a more specific comment linking to the source file and version of the GORM driver from which this logic was copied. This will serve as a clear reminder for future developers to verify this implementation when upgrading the GORM dependency.

✅ Security Check Passed

No security issues found – changes LGTM.

\n\n

Architecture Issues (1)

Severity Location Issue
🟡 Warning pumps/sql.go:130-132
The custom PostgreSQL connection logic manually replicates DSN parsing (specifically for timezone extraction) from the `gorm.io/driver/postgres` library. While necessary for this fix, it creates a maintenance dependency. If the GORM driver's DSN parsing logic changes in a future version, this implementation will become outdated and could lead to behavioral inconsistencies.
💡 SuggestionTo mitigate this, add a more specific comment linking to the source file and version of the GORM driver from which this logic was copied. This will serve as a clear reminder for future developers to verify this implementation when upgrading the GORM dependency.
\n\n \n\n

✅ Quality Check Passed

No quality issues found – changes LGTM.


Powered by Visor from Probelabs

Last updated: 2026-04-17T12:06:05.466Z | Triggered by: pr_updated | Commit: a66909a

💡 TIP: You can chat with Visor using /visor ask <your question>

@buraksezer
Copy link
Copy Markdown
Contributor Author

Quality Gate Failed Quality Gate failed

Failed conditions 0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

This is misleading. TestSQLWriteData_PreferSimpleProtocol_Month covers and tests the new code.

@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@buraksezer buraksezer closed this Apr 17, 2026
@buraksezer
Copy link
Copy Markdown
Contributor Author

Closed this PR to make the QA process easier. The bug only appears with this go.mod file: https://github.com/TykTechnologies/tyk-pump/pull/959/changes#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants